[TASK] Add CompoundSelector class - #1488
Conversation
JakeQZ
added
enhancement
refactor
|
Marking as 'draft' for now, as will need to include changes from #1485. |
fa8cc89 to
86a8bbf
Compare
86a8bbf to
328481e
Compare
|
This now includes the changes from #1485 (parsing functionality is duplicated, but the duplication will eventually be removed from |
oliverklee
left a comment
oliverklee
left a comment
There was a problem hiding this comment.
Looks generally very good! Just some nits.
Also, this should get a changelog entry (and maybe we can add the changelog entry for #1487 as well while we're at it).
| $selectorParts = []; | ||
| $stringWrapperCharacter = null; | ||
| $functionNestingLevel = 0; | ||
| static $stopCharacters = [ |
There was a problem hiding this comment.
Would it make sense to move this to a class constant instead? (In general, I'm not a big fan of static local variables as these are not constant and hence might introduce hidden state.)
There was a problem hiding this comment.
My thinking was that having them in the method makes it easier to check that the cases in the switch statement cover them all. There are arguments either way; I've moved them to a constant.
| { | ||
| return [ | ||
| 'escaped double quote in double-quoted attribute' => ['a[href="test\\"value"]'], | ||
| 'escaped single quote in single-quoted attribute' => ['a[href=\'test\\\'value\']'], |
There was a problem hiding this comment.
For the cases with escaped single quotes, I propose we use double quotes for the string itself, which would reduce additional quoting and make the data easier to read (for humans).
There was a problem hiding this comment.
I've changed the represntation of the strings that contain only single quotes.
I read somewhere that using double quotes to enclose strings no longer has any impact on performance. If that's true, it may even be the case that using double quotes to enclose strings containing single quotes improves performance, since the interpreter doesn't have to deal with unescaping.
I was planning to do that when the new classes are actually used in the |
3 participants
Part of #1325.